Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full support for a new Qwen CustomVoice TTS engine across frontend and backend: UI options and selection logic, profile validation and preset voice handling, API type/schema updates, a new QwenCustomVoice backend with async model lifecycle, model registry changes, and build/package adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (UI/Form)
participant API as Server API
participant Routes as Generations Route
participant Profiles as Profile Service
participant BackendFactory as get_tts_backend_for_engine()
participant Engine as QwenCustomVoiceBackend
UI->>API: POST /generate (text, profile_id, engine?)
API->>Routes: handle request
Routes->>Profiles: load profile, _resolve_generation_engine(...)
Routes->>Profiles: validate_profile_engine(profile, engine)
Routes->>BackendFactory: get_tts_backend_for_engine(engine)
BackendFactory->>Engine: select/construct QwenCustomVoiceBackend
Routes->>Engine: generate(text, voice_prompt, language, instruct)
Engine->>Engine: ensure model loaded (load_model_async if needed)
Engine->>Engine: run generation in background thread
Engine-->>Routes: return waveform + sample_rate
Routes-->>API: stream/send generated audio
API-->>UI: audio response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
app/src/components/VoiceProfiles/ProfileCard.tsx (1)
106-115: Minor: Potential display ofundefinedifpreset_engineis missing.If a preset profile somehow lacks a
preset_enginevalue, the badge would displayundefined. This is unlikely in practice but could be guarded:{profile.voice_type === 'preset' && ( <Badge variant="secondary" className="text-xs h-5 px-1.5"> - {ENGINE_DISPLAY_NAMES[profile.preset_engine ?? ''] ?? profile.preset_engine} + {ENGINE_DISPLAY_NAMES[profile.preset_engine ?? ''] ?? profile.preset_engine ?? 'Preset'} </Badge> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/VoiceProfiles/ProfileCard.tsx` around lines 106 - 115, The badge rendering in ProfileCard (where profile.voice_type is 'preset') can show "undefined" if profile.preset_engine is missing; update the display logic in the ProfileCard component to safely fallback to a friendly string (e.g., 'unknown' or 'preset') when ENGINE_DISPLAY_NAMES[profile.preset_engine ?? ''] and profile.preset_engine are both falsy. Locate the conditional that uses ENGINE_DISPLAY_NAMES and profile.preset_engine and change the fallback expression so it never returns undefined (use a final default string).app/src/lib/hooks/useGenerationForm.ts (1)
96-100: Consider adding a fallback formodelSizewhen constructing the model name.For
qwen_custom_voice, the model name is constructed asqwen-custom-voice-${data.modelSize}. IfmodelSizeis undefined (e.g., not explicitly set), this producesqwen-custom-voice-undefined. The form has a default of'1.7B'(line 63), but if the schema allowsmodelSizeto be optional, a defensive fallback would be safer.💡 Suggested improvement
: engine === 'qwen_custom_voice' - ? `qwen-custom-voice-${data.modelSize}` + ? `qwen-custom-voice-${data.modelSize || '1.7B'}` : `qwen-tts-${data.modelSize}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/hooks/useGenerationForm.ts` around lines 96 - 100, When building the model string in useGenerationForm (the ternary that returns `qwen-custom-voice-${data.modelSize}`), guard against an undefined modelSize by using a fallback (e.g., `data.modelSize ?? '1.7B'` or the form's default) so the constructed name never becomes `qwen-custom-voice-undefined`; update the branch that handles `engine === 'qwen_custom_voice'` to interpolate that fallback value instead of `data.modelSize` directly.backend/build_binary.py (1)
89-91: Consider adding a section comment for the Qwen CustomVoice backend.The hidden import for
qwen_custom_voice_backendis inserted without a comment, unlike other backend additions (e.g., Kokoro at lines 233-247 has a descriptive comment). This makes it harder to identify during maintenance.💡 Suggested improvement
"--hidden-import", "backend.backends.pytorch_backend", + # Qwen CustomVoice — preset speaker engine using Qwen3-TTS "--hidden-import", "backend.backends.qwen_custom_voice_backend",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/build_binary.py` around lines 89 - 91, Add a short descriptive section comment above the Qwen CustomVoice backend hidden-import lines (the entries containing "backend.backends.qwen_custom_voice_backend" and "backend.utils.audio") to match the style used for other backends (e.g., the Kokoro comment block). Place a one-line comment like "# Qwen CustomVoice backend" immediately before those list entries so maintainers can easily identify this backend group.backend/voicebox-server.spec (1)
8-8: Consider splitting the longhiddenimportslist for maintainability.The single-line
hiddenimportslist is difficult to read and diff. While this may be auto-generated, if manually maintained, consider splitting into multiple lines or groups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/voicebox-server.spec` at line 8, The single-line hiddenimports assignment is hard to read and maintain; update the hiddenimports variable to span multiple lines as a Python list literal (e.g., broken into several lines or grouped by category such as backend.*, backend.services.*, external libs) while keeping the same symbol name hiddenimports and preserving all string entries and commas so the value is unchanged; ensure the resulting formatting is a valid Python list literal (opening [, each entry on its own line or grouped lines, trailing comma allowed, closing ]) so diffs are smaller and grouping/ordering (optionally alphabetical) is easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Around line 133-143: The type assertion used when setting the engine from
selectedProfile?.default_engine in the form.setValue('engine', ...) call is
missing the 'qwen_custom_voice' variant; update the union of allowed engine
string literals to include 'qwen_custom_voice' so the cast covers that value
(i.e., ensure the same union used in the setValue call includes
'qwen_custom_voice' alongside 'qwen', 'luxtts', 'chatterbox',
'chatterbox_turbo', 'tada', and 'kokoro').
In `@app/src/components/VoiceProfiles/ProfileForm.tsx`:
- Around line 490-509: When creating a preset profile in the voiceSource ===
'builtin' branch, additionally validate that the selectedPresetVoiceId actually
belongs to the currently selectedPresetEngine before calling
createProfile.mutateAsync; lookup the selected voice from whatever collection
maps voices to engines (e.g., voices list, voicesByEngine or presetVoicesMap
used elsewhere in ProfileForm) and if the voice is missing or its engine !==
selectedPresetEngine, show a destructive toast (like the existing "No voice
selected" toast) indicating the voice/engine mismatch and return early,
otherwise proceed to call createProfile.mutateAsync with the validated
selectedPresetEngine and selectedPresetVoiceId.
- Around line 441-448: The update payload currently uses default_engine:
defaultEngine || undefined which omits the field when defaultEngine is '',
preventing the backend (backend/services/profiles.py) from clearing the existing
default; change the payload in the updateProfile.mutateAsync call so that when
defaultEngine is the explicit "No preference" empty string it sends a present
null (e.g., default_engine: defaultEngine === '' ? null : defaultEngine ||
undefined) so the backend can detect the field and clear the stored
default_engine for the profile identified by editingProfileId.
In `@backend/backends/kokoro_backend.py`:
- Around line 177-191: The _get_pipeline method currently falls back to "a" for
unknown lang_code via LANG_CODE_MAP.get(lang_code, "a"), which silently routes
unsupported languages to the US English pipeline; change the logic in
_get_pipeline to first check if lang_code exists in LANG_CODE_MAP and if not
raise a clear exception (e.g., ValueError) indicating the language is
unsupported by Kokoro, otherwise map with LANG_CODE_MAP[lang_code] and proceed
to create/return the KPipeline; update any callers if they expect silent
fallback to handle the raised exception.
- Around line 216-225: The current code path that returns
{"voice_type":"preset","preset_engine":"kokoro","preset_voice_id":KOKORO_DEFAULT_VOICE},
False silently substitutes the default voice for cloned profiles and must
instead reject cloned-profile requests as unsupported; change that code (the
method that currently returns the dict + False) to detect a cloned profile and
raise or return an explicit unsupported error (e.g., raise
UnsupportedProfileError or NotImplementedError with a clear message) rather than
returning KOKORO_DEFAULT_VOICE, and ensure callers handle that error path
instead of treating the request as successful.
In `@backend/database/migrations.py`:
- Around line 185-226: The _resolve_relative_paths function currently uses
Path(path_val).resolve() which resolves against the process CWD and can produce
incorrect results in production; instead obtain the configured data directory
(e.g., from your application's config/init_db context—something like
settings.data_dir or the same source used when writing these paths) and resolve
relative entries against that directory (use Path(data_dir) / path_val or
Path(data_dir).joinpath(path_val) then .resolve()), verify that this resolved
path exists before updating via the same conn.execute(...) UPDATE used now, keep
the existing checks so absolute paths remain untouched, and ensure data_dir is
normalized to an absolute Path and handled gracefully if missing.
In `@backend/models.py`:
- Around line 18-22: Add validation to the Pydantic model so malformed
non-cloning profiles are rejected: implement validators (e.g., `@root_validator`
or `@validator` on voice_type) that (1) when voice_type == "preset" require
non-empty preset_engine and preset_voice_id, (2) when voice_type == "designed"
reject or require design_prompt only if the backend supports it (currently
reject/desist if unsupported), and (3) validate preset_engine and default_engine
against a canonical set of supported engine identifiers (use a shared
SUPPORTED_ENGINES constant or enum) instead of allowing arbitrary strings;
update the model fields voice_type, preset_engine, preset_voice_id,
design_prompt, default_engine to use these validators so invalid combinations
fail validation immediately.
In `@backend/routes/profiles.py`:
- Around line 147-156: The TOCTOU checks around DBVoiceProfile (checking
preset_engine/preset_voice_id and name with .first()) are racy; instead add a
database-level unique constraint on (preset_engine, preset_voice_id) in the
DBVoiceProfile model and stop relying on those pre-checks in the preset-seeding
route, then perform inserts and either use a DB upsert/ON CONFLICT DO NOTHING
(preferred) or catch sqlalchemy.exc.IntegrityError around the session add/commit
and ignore conflicts so the seeding becomes idempotent under concurrent
requests; apply the same change for both uniqueness checks currently guarding
name and the (preset_engine,preset_voice_id) insert logic.
- Around line 159-168: The seeded Kokoro DBVoiceProfile entries are missing
default_engine="kokoro", causing engine selection to remain set to a previous
backend; update the DBVoiceProfile construction for these Kokoro presets (the
DBVoiceProfile instantiation that sets preset_engine="kokoro" and
preset_voice_id=voice_id) to include default_engine="kokoro" so seeded profiles
explicitly set the engine. Ensure the same change is applied wherever Kokoro
preset profiles are created so selection routes to the kokoro backend.
In `@backend/services/profiles.py`:
- Around line 429-435: The preset branch blindly returns the profile's
preset_engine/preset_voice_id ignoring the requested engine; update the
voice_type == "preset" handling to check that the requested engine (engine)
matches profile.preset_engine and fail fast if it doesn't (raise a clear
exception or return an error) rather than returning the preset_voice_id to the
wrong backend; keep returning the existing dict when engines match and include a
descriptive message referencing profile.preset_engine and preset_voice_id when
rejecting the request.
---
Nitpick comments:
In `@app/src/components/VoiceProfiles/ProfileCard.tsx`:
- Around line 106-115: The badge rendering in ProfileCard (where
profile.voice_type is 'preset') can show "undefined" if profile.preset_engine is
missing; update the display logic in the ProfileCard component to safely
fallback to a friendly string (e.g., 'unknown' or 'preset') when
ENGINE_DISPLAY_NAMES[profile.preset_engine ?? ''] and profile.preset_engine are
both falsy. Locate the conditional that uses ENGINE_DISPLAY_NAMES and
profile.preset_engine and change the fallback expression so it never returns
undefined (use a final default string).
In `@app/src/lib/hooks/useGenerationForm.ts`:
- Around line 96-100: When building the model string in useGenerationForm (the
ternary that returns `qwen-custom-voice-${data.modelSize}`), guard against an
undefined modelSize by using a fallback (e.g., `data.modelSize ?? '1.7B'` or the
form's default) so the constructed name never becomes
`qwen-custom-voice-undefined`; update the branch that handles `engine ===
'qwen_custom_voice'` to interpolate that fallback value instead of
`data.modelSize` directly.
In `@backend/build_binary.py`:
- Around line 89-91: Add a short descriptive section comment above the Qwen
CustomVoice backend hidden-import lines (the entries containing
"backend.backends.qwen_custom_voice_backend" and "backend.utils.audio") to match
the style used for other backends (e.g., the Kokoro comment block). Place a
one-line comment like "# Qwen CustomVoice backend" immediately before those list
entries so maintainers can easily identify this backend group.
In `@backend/voicebox-server.spec`:
- Line 8: The single-line hiddenimports assignment is hard to read and maintain;
update the hiddenimports variable to span multiple lines as a Python list
literal (e.g., broken into several lines or grouped by category such as
backend.*, backend.services.*, external libs) while keeping the same symbol name
hiddenimports and preserving all string entries and commas so the value is
unchanged; ensure the resulting formatting is a valid Python list literal
(opening [, each entry on its own line or grouped lines, trailing comma allowed,
closing ]) so diffs are smaller and grouping/ordering (optionally alphabetical)
is easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4460e508-5827-498c-ac47-02a4d11eeeb2
⛔ Files ignored due to path filters (1)
tauri/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
app/src/components/Generation/EngineModelSelector.tsxapp/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/Generation/GenerationForm.tsxapp/src/components/ServerSettings/ModelManagement.tsxapp/src/components/VoiceProfiles/ProfileCard.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/components/VoiceProfiles/ProfileList.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/constants/languages.tsapp/src/lib/hooks/useGenerationForm.tsapp/src/stores/uiStore.tsbackend/backends/__init__.pybackend/backends/kokoro_backend.pybackend/backends/qwen_custom_voice_backend.pybackend/build_binary.pybackend/config.pybackend/database/migrations.pybackend/database/models.pybackend/models.pybackend/requirements.txtbackend/routes/profiles.pybackend/services/profiles.pybackend/voicebox-server.specdocs/content/docs/developer/tts-engines.mdxdocs/notes/PROJECT_STATUS.md
| if (selectedProfile?.default_engine) { | ||
| form.setValue( | ||
| 'engine', | ||
| selectedProfile.default_engine as | ||
| | 'qwen' | ||
| | 'luxtts' | ||
| | 'chatterbox' | ||
| | 'chatterbox_turbo' | ||
| | 'tada' | ||
| | 'kokoro', | ||
| ); |
There was a problem hiding this comment.
Missing 'qwen_custom_voice' in engine type assertion.
The default_engine value is cast to a union type that doesn't include 'qwen_custom_voice', even though this PR adds it as a valid engine. If a preset profile has default_engine: 'qwen_custom_voice', the type assertion would be incorrect.
🔧 Proposed fix
form.setValue(
'engine',
selectedProfile.default_engine as
| 'qwen'
+ | 'qwen_custom_voice'
| 'luxtts'
| 'chatterbox'
| 'chatterbox_turbo'
| 'tada'
| 'kokoro',
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/Generation/FloatingGenerateBox.tsx` around lines 133 -
143, The type assertion used when setting the engine from
selectedProfile?.default_engine in the form.setValue('engine', ...) call is
missing the 'qwen_custom_voice' variant; update the union of allowed engine
string literals to include 'qwen_custom_voice' so the cast covers that value
(i.e., ensure the same union used in the setValue call includes
'qwen_custom_voice' alongside 'qwen', 'luxtts', 'chatterbox',
'chatterbox_turbo', 'tada', and 'kokoro').
| await updateProfile.mutateAsync({ | ||
| profileId: editingProfileId, | ||
| data: { | ||
| name: data.name, | ||
| description: data.description, | ||
| language: data.language, | ||
| default_engine: defaultEngine || undefined, | ||
| }, |
There was a problem hiding this comment.
This payload can't clear an existing default engine.
Selecting “No preference” sets defaultEngine to '', but defaultEngine || undefined drops the field from the request. backend/services/profiles.py only clears default_engine when the field is present, so the old value is silently kept.
Suggested fix
await updateProfile.mutateAsync({
profileId: editingProfileId,
data: {
name: data.name,
description: data.description,
language: data.language,
- default_engine: defaultEngine || undefined,
+ default_engine: defaultEngine,
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await updateProfile.mutateAsync({ | |
| profileId: editingProfileId, | |
| data: { | |
| name: data.name, | |
| description: data.description, | |
| language: data.language, | |
| default_engine: defaultEngine || undefined, | |
| }, | |
| await updateProfile.mutateAsync({ | |
| profileId: editingProfileId, | |
| data: { | |
| name: data.name, | |
| description: data.description, | |
| language: data.language, | |
| default_engine: defaultEngine, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/VoiceProfiles/ProfileForm.tsx` around lines 441 - 448, The
update payload currently uses default_engine: defaultEngine || undefined which
omits the field when defaultEngine is '', preventing the backend
(backend/services/profiles.py) from clearing the existing default; change the
payload in the updateProfile.mutateAsync call so that when defaultEngine is the
explicit "No preference" empty string it sends a present null (e.g.,
default_engine: defaultEngine === '' ? null : defaultEngine || undefined) so the
backend can detect the field and clear the stored default_engine for the profile
identified by editingProfileId.
| } else if (voiceSource === 'builtin') { | ||
| // Creating preset profile from built-in voice | ||
| if (!selectedPresetVoiceId) { | ||
| toast({ | ||
| title: 'No voice selected', | ||
| description: 'Please select a built-in voice.', | ||
| variant: 'destructive', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const profile = await createProfile.mutateAsync({ | ||
| name: data.name, | ||
| description: data.description, | ||
| language: data.language, | ||
| voice_type: 'preset' as VoiceType, | ||
| preset_engine: selectedPresetEngine, | ||
| preset_voice_id: selectedPresetVoiceId, | ||
| default_engine: selectedPresetEngine, | ||
| }); |
There was a problem hiding this comment.
Validate the selected preset against the current engine before create.
This only checks that selectedPresetVoiceId is non-empty. After switching engines, that ID can still belong to the previous engine, and this branch will persist an invalid (preset_engine, preset_voice_id) pair.
Suggested fix
- if (!selectedPresetVoiceId) {
+ const selectedPreset = presetVoices.find(
+ (voice: PresetVoice) => voice.voice_id === selectedPresetVoiceId,
+ );
+ if (!selectedPreset) {
toast({
title: 'No voice selected',
- description: 'Please select a built-in voice.',
+ description: 'Please select a built-in voice for the chosen engine.',
variant: 'destructive',
});
return;
}
const profile = await createProfile.mutateAsync({
name: data.name,
description: data.description,
language: data.language,
voice_type: 'preset' as VoiceType,
preset_engine: selectedPresetEngine,
- preset_voice_id: selectedPresetVoiceId,
+ preset_voice_id: selectedPreset.voice_id,
default_engine: selectedPresetEngine,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (voiceSource === 'builtin') { | |
| // Creating preset profile from built-in voice | |
| if (!selectedPresetVoiceId) { | |
| toast({ | |
| title: 'No voice selected', | |
| description: 'Please select a built-in voice.', | |
| variant: 'destructive', | |
| }); | |
| return; | |
| } | |
| const profile = await createProfile.mutateAsync({ | |
| name: data.name, | |
| description: data.description, | |
| language: data.language, | |
| voice_type: 'preset' as VoiceType, | |
| preset_engine: selectedPresetEngine, | |
| preset_voice_id: selectedPresetVoiceId, | |
| default_engine: selectedPresetEngine, | |
| }); | |
| } else if (voiceSource === 'builtin') { | |
| // Creating preset profile from built-in voice | |
| const selectedPreset = presetVoices.find( | |
| (voice: PresetVoice) => voice.voice_id === selectedPresetVoiceId, | |
| ); | |
| if (!selectedPreset) { | |
| toast({ | |
| title: 'No voice selected', | |
| description: 'Please select a built-in voice for the chosen engine.', | |
| variant: 'destructive', | |
| }); | |
| return; | |
| } | |
| const profile = await createProfile.mutateAsync({ | |
| name: data.name, | |
| description: data.description, | |
| language: data.language, | |
| voice_type: 'preset' as VoiceType, | |
| preset_engine: selectedPresetEngine, | |
| preset_voice_id: selectedPreset.voice_id, | |
| default_engine: selectedPresetEngine, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/VoiceProfiles/ProfileForm.tsx` around lines 490 - 509,
When creating a preset profile in the voiceSource === 'builtin' branch,
additionally validate that the selectedPresetVoiceId actually belongs to the
currently selectedPresetEngine before calling createProfile.mutateAsync; lookup
the selected voice from whatever collection maps voices to engines (e.g., voices
list, voicesByEngine or presetVoicesMap used elsewhere in ProfileForm) and if
the voice is missing or its engine !== selectedPresetEngine, show a destructive
toast (like the existing "No voice selected" toast) indicating the voice/engine
mismatch and return early, otherwise proceed to call createProfile.mutateAsync
with the validated selectedPresetEngine and selectedPresetVoiceId.
| def _get_pipeline(self, lang_code: str): | ||
| """Get or create a KPipeline for the given language code.""" | ||
| kokoro_lang = LANG_CODE_MAP.get(lang_code, "a") | ||
|
|
||
| if kokoro_lang not in self._pipelines: | ||
| from kokoro import KPipeline | ||
|
|
||
| # Create pipeline with our existing model (no redundant model loading) | ||
| self._pipelines[kokoro_lang] = KPipeline( | ||
| lang_code=kokoro_lang, | ||
| repo_id=KOKORO_HF_REPO, | ||
| model=self._model, | ||
| ) | ||
|
|
||
| return self._pipelines[kokoro_lang] |
There was a problem hiding this comment.
Reject unsupported Kokoro languages instead of defaulting to English.
LANG_CODE_MAP.get(lang_code, "a") silently converts unsupported requests to American English. GenerationRequest still accepts languages Kokoro does not support, so kokoro requests for de, he, etc. will synthesize with the wrong pipeline instead of failing fast.
Suggested fix
def _get_pipeline(self, lang_code: str):
"""Get or create a KPipeline for the given language code."""
- kokoro_lang = LANG_CODE_MAP.get(lang_code, "a")
+ if lang_code not in LANG_CODE_MAP:
+ raise ValueError(f"Kokoro does not support language '{lang_code}'")
+ kokoro_lang = LANG_CODE_MAP[lang_code]
if kokoro_lang not in self._pipelines:
from kokoro import KPipeline🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/backends/kokoro_backend.py` around lines 177 - 191, The _get_pipeline
method currently falls back to "a" for unknown lang_code via
LANG_CODE_MAP.get(lang_code, "a"), which silently routes unsupported languages
to the US English pipeline; change the logic in _get_pipeline to first check if
lang_code exists in LANG_CODE_MAP and if not raise a clear exception (e.g.,
ValueError) indicating the language is unsupported by Kokoro, otherwise map with
LANG_CODE_MAP[lang_code] and proceed to create/return the KPipeline; update any
callers if they expect silent fallback to handle the raised exception.
| Kokoro doesn't do traditional voice cloning from arbitrary audio. | ||
| When called for a cloned profile (fallback), uses the default voice. | ||
| For preset profiles, the voice_prompt dict is built by the profile | ||
| service and bypasses this method entirely. | ||
| """ | ||
| return { | ||
| "voice_type": "preset", | ||
| "preset_engine": "kokoro", | ||
| "preset_voice_id": KOKORO_DEFAULT_VOICE, | ||
| }, False |
There was a problem hiding this comment.
Don't silently substitute af_heart for cloned profiles.
When a cloned profile is generated through Kokoro, this ignores the uploaded reference audio and always uses the default preset voice. That produces the wrong voice while looking successful; this path should reject the request as unsupported.
Suggested fix
async def create_voice_prompt(
self,
audio_path: str,
reference_text: str,
use_cache: bool = True,
) -> tuple[dict, bool]:
@@
- return {
- "voice_type": "preset",
- "preset_engine": "kokoro",
- "preset_voice_id": KOKORO_DEFAULT_VOICE,
- }, False
+ raise ValueError("Kokoro only supports preset voices; cloned profiles are not supported")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Kokoro doesn't do traditional voice cloning from arbitrary audio. | |
| When called for a cloned profile (fallback), uses the default voice. | |
| For preset profiles, the voice_prompt dict is built by the profile | |
| service and bypasses this method entirely. | |
| """ | |
| return { | |
| "voice_type": "preset", | |
| "preset_engine": "kokoro", | |
| "preset_voice_id": KOKORO_DEFAULT_VOICE, | |
| }, False | |
| Kokoro doesn't do traditional voice cloning from arbitrary audio. | |
| When called for a cloned profile (fallback), uses the default voice. | |
| For preset profiles, the voice_prompt dict is built by the profile | |
| service and bypasses this method entirely. | |
| """ | |
| raise ValueError("Kokoro only supports preset voices; cloned profiles are not supported") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/backends/kokoro_backend.py` around lines 216 - 225, The current code
path that returns
{"voice_type":"preset","preset_engine":"kokoro","preset_voice_id":KOKORO_DEFAULT_VOICE},
False silently substitutes the default voice for cloned profiles and must
instead reject cloned-profile requests as unsupported; change that code (the
method that currently returns the dict + False) to detect a cloned profile and
raise or return an explicit unsupported error (e.g., raise
UnsupportedProfileError or NotImplementedError with a clear message) rather than
returning KOKORO_DEFAULT_VOICE, and ensure callers handle that error path
instead of treating the request as successful.
| voice_type: Optional[str] = Field(default="cloned", pattern="^(cloned|preset|designed)$") | ||
| preset_engine: Optional[str] = Field(None, max_length=50) | ||
| preset_voice_id: Optional[str] = Field(None, max_length=100) | ||
| design_prompt: Optional[str] = Field(None, max_length=2000) | ||
| default_engine: Optional[str] = Field(None, max_length=50) |
There was a problem hiding this comment.
Reject malformed non-cloning profiles at validation time.
This schema now accepts voice_type="preset" without preset_engine/preset_voice_id, arbitrary engine strings in preset_engine/default_engine, and voice_type="designed" even though current generation code has no backend path for design_prompt. Those rows save successfully and only break later during prompt creation/generation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/models.py` around lines 18 - 22, Add validation to the Pydantic model
so malformed non-cloning profiles are rejected: implement validators (e.g.,
`@root_validator` or `@validator` on voice_type) that (1) when voice_type ==
"preset" require non-empty preset_engine and preset_voice_id, (2) when
voice_type == "designed" reject or require design_prompt only if the backend
supports it (currently reject/desist if unsupported), and (3) validate
preset_engine and default_engine against a canonical set of supported engine
identifiers (use a shared SUPPORTED_ENGINES constant or enum) instead of
allowing arbitrary strings; update the model fields voice_type, preset_engine,
preset_voice_id, design_prompt, default_engine to use these validators so
invalid combinations fail validation immediately.
backend/routes/profiles.py
Outdated
| existing = ( | ||
| db.query(DBVoiceProfile) | ||
| .filter_by(preset_engine="kokoro", preset_voice_id=voice_id) | ||
| .first() | ||
| ) | ||
| if existing: | ||
| continue | ||
|
|
||
| # Skip name collisions | ||
| if db.query(DBVoiceProfile).filter_by(name=profile_name).first(): |
There was a problem hiding this comment.
Make preset seeding idempotent under concurrent requests.
These .first() guards are TOCTOU. If two /seed requests race, the loser can still reach commit() with rows that now violate the unique name constraint, which rolls back the whole batch and returns 500 instead of a clean no-op. Please move the uniqueness guarantee to the DB layer for (preset_engine, preset_voice_id) and ignore/catch conflicts during insert.
Also applies to: 191-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/profiles.py` around lines 147 - 156, The TOCTOU checks around
DBVoiceProfile (checking preset_engine/preset_voice_id and name with .first())
are racy; instead add a database-level unique constraint on (preset_engine,
preset_voice_id) in the DBVoiceProfile model and stop relying on those
pre-checks in the preset-seeding route, then perform inserts and either use a DB
upsert/ON CONFLICT DO NOTHING (preferred) or catch sqlalchemy.exc.IntegrityError
around the session add/commit and ignore conflicts so the seeding becomes
idempotent under concurrent requests; apply the same change for both uniqueness
checks currently guarding name and the (preset_engine,preset_voice_id) insert
logic.
backend/routes/profiles.py
Outdated
| profile = DBVoiceProfile( | ||
| id=str(uuid.uuid4()), | ||
| name=profile_name, | ||
| description=f"Kokoro preset voice — {display_name} ({gender})", | ||
| language=lang, | ||
| voice_type="preset", | ||
| preset_engine="kokoro", | ||
| preset_voice_id=voice_id, | ||
| created_at=datetime.utcnow(), | ||
| updated_at=datetime.utcnow(), |
There was a problem hiding this comment.
Seeded Kokoro profiles need default_engine="kokoro".
Manual preset creation auto-fills this, and the Qwen seeder does too. Without it, selecting one of these seeded profiles can leave the previous engine active and route generation through the wrong backend.
Suggested fix
profile = DBVoiceProfile(
id=str(uuid.uuid4()),
name=profile_name,
description=f"Kokoro preset voice — {display_name} ({gender})",
language=lang,
voice_type="preset",
preset_engine="kokoro",
preset_voice_id=voice_id,
+ default_engine="kokoro",
created_at=datetime.utcnow(),
updated_at=datetime.utcnow(),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| profile = DBVoiceProfile( | |
| id=str(uuid.uuid4()), | |
| name=profile_name, | |
| description=f"Kokoro preset voice — {display_name} ({gender})", | |
| language=lang, | |
| voice_type="preset", | |
| preset_engine="kokoro", | |
| preset_voice_id=voice_id, | |
| created_at=datetime.utcnow(), | |
| updated_at=datetime.utcnow(), | |
| profile = DBVoiceProfile( | |
| id=str(uuid.uuid4()), | |
| name=profile_name, | |
| description=f"Kokoro preset voice — {display_name} ({gender})", | |
| language=lang, | |
| voice_type="preset", | |
| preset_engine="kokoro", | |
| preset_voice_id=voice_id, | |
| default_engine="kokoro", | |
| created_at=datetime.utcnow(), | |
| updated_at=datetime.utcnow(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/profiles.py` around lines 159 - 168, The seeded Kokoro
DBVoiceProfile entries are missing default_engine="kokoro", causing engine
selection to remain set to a previous backend; update the DBVoiceProfile
construction for these Kokoro presets (the DBVoiceProfile instantiation that
sets preset_engine="kokoro" and preset_voice_id=voice_id) to include
default_engine="kokoro" so seeded profiles explicitly set the engine. Ensure the
same change is applied wherever Kokoro preset profiles are created so selection
routes to the kokoro backend.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/src/components/VoiceProfiles/ProfileForm.tsx (1)
476-483:⚠️ Potential issue | 🟠 MajorSend an explicit clear value for “No preference”.
When
defaultEngineis'', this dropsdefault_enginefrom the update payload instead of clearing it, so editing a profile cannot remove an existing default engine.Suggested fix
await updateProfile.mutateAsync({ profileId: editingProfileId, data: { name: data.name, description: data.description, language: data.language, - default_engine: defaultEngine || undefined, + default_engine: defaultEngine === '' ? null : defaultEngine || undefined, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/VoiceProfiles/ProfileForm.tsx` around lines 476 - 483, The update payload currently omits default_engine when defaultEngine is an empty string so clearing a profile's default isn't possible; in the call to updateProfile.mutateAsync (the block using editingProfileId and data { name, description, language, default_engine }), send an explicit clear value instead of dropping the key — e.g. set default_engine to null when defaultEngine === '' and otherwise to defaultEngine (default_engine: defaultEngine === '' ? null : defaultEngine) so "No preference" clears the saved engine.
🧹 Nitpick comments (3)
backend/voicebox-server.spec (1)
8-8: Addcollect_all('kokoro')for consistency with other backend packages.The
kokoro.*modules are explicitly listed in hiddenimports on line 8, but unlikeqwen_tts,misaki,language_tags, andespeakng_loader, there's no correspondingcollect_all('kokoro')call. This inconsistency means any data files or binaries shipped with thekokoropackage won't be bundled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/voicebox-server.spec` at line 8, Add a collect_all('kokoro') call in the spec so kokoro package data/binaries are bundled like other backends; update the spec near the hiddenimports list (which already includes 'kokoro', 'kokoro.pipeline', 'kokoro.model', etc.) to call collect_all('kokoro') alongside existing collect_all calls (e.g., those for qwen_tts/misaki) so kokoro's non-Python assets are included in the build.backend/database/migrations.py (1)
37-37: This adds an O(total rows) startup scan to every launch.
run_migrations()executes on each startup, and_resolve_relative_paths()currently reads every non-null path row from four tables before filtering in Python. On larger libraries, that becomes permanent startup work even after all rows have already been normalized. Consider gating this behind a one-shot schema/version flag or narrowing the SQL to relative-path candidates instead offetchall()-ing the full set every time.Also applies to: 216-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/database/migrations.py` at line 37, run_migrations() currently calls _resolve_relative_paths() which fetches all non-null path rows from multiple tables every startup; change this so we avoid an O(total rows) scan by either gating _resolve_relative_paths() behind a one-shot schema/version flag stored in your migrations table (so it runs only once) or by changing the SQL in _resolve_relative_paths() to only select candidates that are relative paths (e.g., add WHERE clauses like path NOT LIKE '/%' or other DB-specific checks) instead of fetchall()'ing entire tables; update the logic in _resolve_relative_paths() and any callers (run_migrations(), and the similar code noted around the other occurrences) to use the gate or narrowed queries and mark the migration as applied so it won't run on subsequent startups.app/src/components/Generation/EngineModelSelector.tsx (1)
45-50: Centralize engine/profile compatibility rules.This file and Lines 62-71 / 275-277 of
app/src/components/VoiceProfiles/ProfileForm.tsxnow maintain inverse hardcoded engine sets. They currently match, but the next engine addition only needs one side to drift for the profile form to allow a default that this selector immediately auto-corrects away. Please move the compatibility rules into one shared source and derive both filters from it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/EngineModelSelector.tsx` around lines 45 - 50, There are duplicated, inverse hardcoded engine sets between CLONING_ENGINES in EngineModelSelector and the profile form filter; create a single shared source (e.g., export a constant or helper) that defines engine<->profile compatibility and import it where needed so both getAvailableOptions (in EngineModelSelector) and the filtering logic in ProfileForm.tsx use the same rule; specifically, move the compatibility data and/or a helper like isProfileCompatibleWithEngine into a new/shared module and update references to ENGINE_OPTIONS, CLONING_ENGINES, getAvailableOptions and the ProfileForm filtering code to call the single exported helper or use the exported set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/Generation/GenerationForm.tsx`:
- Around line 30-35: getEngineSelectValue currently maps the bare 'tada' profile
value to 'tada:1B' which forces applyEngineSelection to reset non-English
profiles to English; update getEngineSelectValue to accept the profile language
(e.g., add a second parameter like language: string | undefined) and return
'tada:1B' only when language === 'en' (or undefined) and return 'tada:3B' when
language !== 'en'; update any callers (such as where applyEngineSelection is
used) to pass the profile's language so the helper can choose the correct TADA
size and avoid overwriting the saved language.
In `@app/src/components/VoiceProfiles/ProfileForm.tsx`:
- Around line 854-859: The language Select is currently uncontrolled (uses
defaultValue={field.value}) so programmatic updates via setSelectedPresetVoiceId
and form.setValue('language', ...) don't reflect; change the Select to be
controlled by replacing defaultValue with value={field.value} and ensure its
onChange calls form.setValue('language', ...) (or use the existing Controller
render's onChange hookup) so that updates from setSelectedPresetVoiceId and
form.setValue update the visible Select value.
In `@backend/routes/generations.py`:
- Around line 23-24: The resolver _resolve_generation_engine is never seeing an
omitted engine because models.GenerationRequest.engine currently defaults to
"qwen"; change GenerationRequest.engine's default to None in backend/models.py
so omission is distinguishable and the existing resolver can fall through to
profile.default_engine / profile.preset_engine, or alternatively modify
_resolve_generation_engine to treat a special sentinel (e.g., None) as “omitted”
and only use the literal "qwen" as the final fallback after checking
profile.default_engine and profile.preset_engine; update references to
GenerationRequest.engine, _resolve_generation_engine, profile.default_engine and
profile.preset_engine accordingly.
- Around line 43-46: The ValueError raised by profiles.validate_profile_engine
is currently caught and re-raised as an HTTPException without chaining, which
loses the original exception context; update both exception handlers that catch
ValueError from profiles.validate_profile_engine to re-raise the HTTPException
using "raise HTTPException(status_code=400, detail=str(e)) from e" so the
original ValueError is preserved in the exception chain (refer to the try/except
blocks around profiles.validate_profile_engine and the HTTPException raises).
---
Duplicate comments:
In `@app/src/components/VoiceProfiles/ProfileForm.tsx`:
- Around line 476-483: The update payload currently omits default_engine when
defaultEngine is an empty string so clearing a profile's default isn't possible;
in the call to updateProfile.mutateAsync (the block using editingProfileId and
data { name, description, language, default_engine }), send an explicit clear
value instead of dropping the key — e.g. set default_engine to null when
defaultEngine === '' and otherwise to defaultEngine (default_engine:
defaultEngine === '' ? null : defaultEngine) so "No preference" clears the saved
engine.
---
Nitpick comments:
In `@app/src/components/Generation/EngineModelSelector.tsx`:
- Around line 45-50: There are duplicated, inverse hardcoded engine sets between
CLONING_ENGINES in EngineModelSelector and the profile form filter; create a
single shared source (e.g., export a constant or helper) that defines
engine<->profile compatibility and import it where needed so both
getAvailableOptions (in EngineModelSelector) and the filtering logic in
ProfileForm.tsx use the same rule; specifically, move the compatibility data
and/or a helper like isProfileCompatibleWithEngine into a new/shared module and
update references to ENGINE_OPTIONS, CLONING_ENGINES, getAvailableOptions and
the ProfileForm filtering code to call the single exported helper or use the
exported set.
In `@backend/database/migrations.py`:
- Line 37: run_migrations() currently calls _resolve_relative_paths() which
fetches all non-null path rows from multiple tables every startup; change this
so we avoid an O(total rows) scan by either gating _resolve_relative_paths()
behind a one-shot schema/version flag stored in your migrations table (so it
runs only once) or by changing the SQL in _resolve_relative_paths() to only
select candidates that are relative paths (e.g., add WHERE clauses like path NOT
LIKE '/%' or other DB-specific checks) instead of fetchall()'ing entire tables;
update the logic in _resolve_relative_paths() and any callers (run_migrations(),
and the similar code noted around the other occurrences) to use the gate or
narrowed queries and mark the migration as applied so it won't run on subsequent
startups.
In `@backend/voicebox-server.spec`:
- Line 8: Add a collect_all('kokoro') call in the spec so kokoro package
data/binaries are bundled like other backends; update the spec near the
hiddenimports list (which already includes 'kokoro', 'kokoro.pipeline',
'kokoro.model', etc.) to call collect_all('kokoro') alongside existing
collect_all calls (e.g., those for qwen_tts/misaki) so kokoro's non-Python
assets are included in the build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8ee5efd-a612-41a6-a33f-e58770033483
📒 Files selected for processing (10)
app/src/components/Generation/EngineModelSelector.tsxapp/src/components/Generation/GenerationForm.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/lib/api/client.tsbackend/database/migrations.pybackend/requirements.txtbackend/routes/generations.pybackend/routes/profiles.pybackend/services/profiles.pybackend/voicebox-server.spec
✅ Files skipped from review due to trivial changes (1)
- backend/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/lib/api/client.ts
- backend/routes/profiles.py
- backend/services/profiles.py
| function getEngineSelectValue(engine: string): string { | ||
| if (engine === 'qwen') return 'qwen:1.7B'; | ||
| if (engine === 'qwen_custom_voice') return 'qwen_custom_voice:1.7B'; | ||
| if (engine === 'tada') return 'tada:1B'; | ||
| return engine; | ||
| } |
There was a problem hiding this comment.
Don’t resolve tada to the English-only model by default.
Profiles only persist default_engine: 'tada', but this helper maps that to tada:1B. For any non-English profile, applyEngineSelection(...) then forces language back to 'en', so selecting the profile loses its saved language. Pick tada:3B when the profile language is not English, or persist the preferred TADA size with the profile.
Suggested fix
-function getEngineSelectValue(engine: string): string {
+function getEngineSelectValue(engine: string, profileLanguage?: string): string {
if (engine === 'qwen') return 'qwen:1.7B';
if (engine === 'qwen_custom_voice') return 'qwen_custom_voice:1.7B';
- if (engine === 'tada') return 'tada:1B';
+ if (engine === 'tada') return profileLanguage === 'en' ? 'tada:1B' : 'tada:3B';
return engine;
}
@@
- applyEngineSelection(form, getEngineSelectValue(preferredEngine));
+ applyEngineSelection(
+ form,
+ getEngineSelectValue(preferredEngine, selectedProfile.language),
+ );Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/Generation/GenerationForm.tsx` around lines 30 - 35,
getEngineSelectValue currently maps the bare 'tada' profile value to 'tada:1B'
which forces applyEngineSelection to reset non-English profiles to English;
update getEngineSelectValue to accept the profile language (e.g., add a second
parameter like language: string | undefined) and return 'tada:1B' only when
language === 'en' (or undefined) and return 'tada:3B' when language !== 'en';
update any callers (such as where applyEngineSelection is used) to pass the
profile's language so the helper can choose the correct TADA size and avoid
overwriting the saved language.
| onClick={() => { | ||
| setSelectedPresetVoiceId(voice.voice_id); | ||
| // Auto-set language from voice | ||
| if (voice.language) { | ||
| form.setValue('language', voice.language as LanguageCode); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
With shadcn/ui Select (Radix Select) and react-hook-form 7, does calling form.setValue(...) update a Select rendered with defaultValue={field.value}, or does the Select need to be controlled with value={field.value}?
💡 Result:
No, calling form.setValue(...) does not reliably update a shadcn/ui Select rendered with defaultValue={field.value}. The Select must be made controlled by using value={field.value} instead. The official shadcn/ui example in the registry (select-form.tsx) uses: <FormField control={form.control} name="email" render={({ field }) => ( Email m@example.com ... )} /> This uses defaultValue, which sets an initial uncontrolled value but does not respond to programmatic updates from form.setValue (or form.reset), as reported in multiple GitHub issues (#7036, #1253, #1463, #549). Users confirm that changing to value={field.value} makes it controlled and responsive to setValue changes. Radix UI Select supports both value (controlled) and defaultValue (uncontrolled) props, so value={field.value} is the correct approach for dynamic form updates with react-hook-form.
Citations:
- 1: [bug]: React hook form Select can not change value shadcn-ui/ui#7036
- 2: Using Select with react-hook-form not setting SelectValue shadcn-ui/ui#1463
- 3: Select with react-hook-form Control shadcn-ui/ui#1253
- 4: https://xgit.ir/rayan/shadcn-ui/src/commit/650b3b9bda2149793aac369e2b9608841a776451/apps/www/registry/new-york/example/select-form.tsx
- 5: Unable to reset the Select component with React Hook Form shadcn-ui/ui#549
🏁 Script executed:
# First, let's locate the ProfileForm.tsx file
fd -e tsx "ProfileForm" --type fRepository: jamiepine/voicebox
Length of output: 110
🏁 Script executed:
# Read lines 854-859 to verify the form.setValue call
sed -n '854,859p' app/src/components/VoiceProfiles/ProfileForm.tsx
# Read lines 1142-1155 to verify the Select component usage
sed -n '1142,1155p' app/src/components/VoiceProfiles/ProfileForm.tsxRepository: jamiepine/voicebox
Length of output: 1139
🏁 Script executed:
# Check if there are other Select components using defaultValue in the file
rg -n "defaultValue=\{field\.value\}" app/src/components/VoiceProfiles/ProfileForm.tsx
# Let's also verify the context around the Select to confirm it's the language field
sed -n '1130,1160p' app/src/components/VoiceProfiles/ProfileForm.tsxRepository: jamiepine/voicebox
Length of output: 1364
Change Select from uncontrolled to controlled to reflect programmatic language updates.
When a built-in voice preset is selected (lines 854–859), form.setValue('language', ...) updates the form state. However, the language <Select> at line 1142 uses defaultValue={field.value}, rendering it uncontrolled. This means the Select won't reflect the programmatic update, showing stale language while the form submits the new one.
Suggested fix
- <Select onValueChange={field.onChange} defaultValue={field.value}>
+ <Select onValueChange={field.onChange} value={field.value}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onClick={() => { | |
| setSelectedPresetVoiceId(voice.voice_id); | |
| // Auto-set language from voice | |
| if (voice.language) { | |
| form.setValue('language', voice.language as LanguageCode); | |
| } | |
| <Select onValueChange={field.onChange} value={field.value}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/VoiceProfiles/ProfileForm.tsx` around lines 854 - 859, The
language Select is currently uncontrolled (uses defaultValue={field.value}) so
programmatic updates via setSelectedPresetVoiceId and form.setValue('language',
...) don't reflect; change the Select to be controlled by replacing defaultValue
with value={field.value} and ensure its onChange calls form.setValue('language',
...) (or use the existing Controller render's onChange hookup) so that updates
from setSelectedPresetVoiceId and form.setValue update the visible Select value.
| try: | ||
| profiles.validate_profile_engine(profile, engine) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the relevant lines
if [ -f "backend/routes/generations.py" ]; then
echo "=== Lines 43-46 ===="
sed -n '43,46p' backend/routes/generations.py
echo ""
echo "=== Lines 240-246 ===="
sed -n '240,246p' backend/routes/generations.py
echo ""
echo "=== Full context around line 43 (40-50) ===="
sed -n '40,50p' backend/routes/generations.py
echo ""
echo "=== Full context around line 243 (240-250) ===="
sed -n '240,250p' backend/routes/generations.py
else
echo "File not found"
fiRepository: jamiepine/voicebox
Length of output: 1422
Explicitly chain exceptions to preserve the original ValueError for debugging.
Both locations (lines 43–46 and 243–246) drop the exception cause, making profile validation errors harder to debug. Use from e to maintain the exception chain:
Fix
except ValueError as e:
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=str(e)) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| profiles.validate_profile_engine(profile, engine) | |
| except ValueError as e: | |
| raise HTTPException(status_code=400, detail=str(e)) | |
| try: | |
| profiles.validate_profile_engine(profile, engine) | |
| except ValueError as e: | |
| raise HTTPException(status_code=400, detail=str(e)) from e |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 46-46: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/generations.py` around lines 43 - 46, The ValueError raised by
profiles.validate_profile_engine is currently caught and re-raised as an
HTTPException without chaining, which loses the original exception context;
update both exception handlers that catch ValueError from
profiles.validate_profile_engine to re-raise the HTTPException using "raise
HTTPException(status_code=400, detail=str(e)) from e" so the original ValueError
is preserved in the exception chain (refer to the try/except blocks around
profiles.validate_profile_engine and the HTTPException raises).
9a9b2b6 to
7980684
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/profiles.py (1)
1-17:⚠️ Potential issue | 🔴 Critical
update_profile_effects()will raise NameError on successful effects update.Line 361 uses
datetime.utcnow()but thedatetimemodule is not imported. Add the import to fix:Suggested fix
+from datetime import datetime🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/profiles.py` around lines 1 - 17, The update_profile_effects function calls datetime.utcnow() but datetime isn't imported, causing a NameError; add the missing import by adding "from datetime import datetime" to the module imports at the top so update_profile_effects can reference datetime.utcnow() (look for the update_profile_effects function and the top import block to place the import).
♻️ Duplicate comments (3)
backend/routes/generations.py (1)
43-46:⚠️ Potential issue | 🟡 MinorPreserve the original validation error in both handlers.
Both
except ValueErrorblocks drop the cause, which makes profile/engine mismatch failures much harder to debug in logs. Re-raise theHTTPExceptionwithfrom e.Suggested fix
except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException(status_code=400, detail=str(e)) from eAlso applies to: 243-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/generations.py` around lines 43 - 46, The current except ValueError handlers around profiles.validate_profile_engine(profile, engine) catch the ValueError and raise an HTTPException but drop the original exception context; update both handlers (the one at the validate_profile_engine call and the similar block at lines around 243-246) to re-raise the HTTPException using "raise HTTPException(... ) from e" so the original ValueError is preserved as the __cause__ for better debugging while keeping the same status_code and detail.app/src/components/VoiceProfiles/ProfileForm.tsx (1)
367-374:⚠️ Potential issue | 🟠 MajorThis auto-clear still won’t persist when editing.
The new effect resets
defaultEngineto''for incompatible choices, but the edit payload still collapses''toundefined. Saving therefore leaves the staledefault_enginein the database, so the invalid engine comes back on the next load.Suggested fix
- default_engine: defaultEngine || undefined, + default_engine: defaultEngine === '' ? null : defaultEngine || undefined,Also applies to: 476-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/VoiceProfiles/ProfileForm.tsx` around lines 367 - 374, The effect clearing defaultEngine to '' prevents selection of an incompatible engine but the save path collapses '' to undefined so the backend retains the old default_engine; update the save/edit payload builder (the function that constructs the edit payload—e.g., the submit/handleSave or updateVoiceProfile code) to explicitly send a null (or an explicit empty value) when defaultEngine === '' (for example set payload.default_engine = defaultEngine === '' ? null : defaultEngine) so the backend will clear the stored value; apply the same change for the other save path referenced around the defaultEngine handling at lines 476-483.app/src/components/Generation/GenerationForm.tsx (1)
30-35:⚠️ Potential issue | 🟠 MajorDon’t resolve bare
tadato the 1B model.This helper still rewrites
default_engine="tada"totada:1B, so selecting a non-English TADA profile resets the form language toen. Choose3Bwhen the profile language is not English, or persist the preferred TADA size with the profile.Suggested fix
-function getEngineSelectValue(engine: string): string { +function getEngineSelectValue(engine: string, profileLanguage?: string): string { if (engine === 'qwen') return 'qwen:1.7B'; if (engine === 'qwen_custom_voice') return 'qwen_custom_voice:1.7B'; - if (engine === 'tada') return 'tada:1B'; + if (engine === 'tada') return profileLanguage === 'en' ? 'tada:1B' : 'tada:3B'; return engine; } @@ - applyEngineSelection(form, getEngineSelectValue(preferredEngine)); + applyEngineSelection( + form, + getEngineSelectValue(preferredEngine, selectedProfile.language), + );Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/GenerationForm.tsx` around lines 30 - 35, Update getEngineSelectValue to stop unconditionally mapping 'tada' to 'tada:1B' — instead accept an optional preferredTadaSize and/or profileLanguage and use those to pick the TADA size: if preferredTadaSize is provided return `tada:{preferredTadaSize}`, else if profileLanguage exists and !== 'en' return 'tada:3B', else fall back to 'tada:1B' (or preserve the bare 'tada' if you prefer to persist the profile value). Apply the same change to the other helper instance mentioned (the second getEngineSelectValue occurrence) so selecting a non-English TADA profile no longer resets language to English.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/backends/qwen_custom_voice_backend.py`:
- Around line 82-93: load/unload/generate currently mutate and read self.model
without synchronization causing race conditions; add a shared asyncio.Lock (or
an active-ops counter with a lock) on the backend class and use it to serialize
model swaps and protect in-flight inference: acquire the lock (or increment
active-ops) in load_model_async and unload_model around mutations to self.model
and _current_model_size, and in generate acquire the lock (or increment
active-ops) briefly to capture a stable reference to self.model into a local
variable which you then pass to _generate_sync; update _generate_sync signature
to accept the model reference so threads use the stable snapshot and ensure the
lock/active-ops is released (or decremented) appropriately to avoid deadlocks.
In `@backend/routes/generations.py`:
- Around line 23-24: Centralize engine-specific model-size fallbacks and use
them whenever engine is auto-selected: add a helper (e.g., _resolve_model_size
or extend _resolve_generation_engine to return both engine and default_size)
that maps engines ("qwen", "tada", etc.) to their correct default model_size
("1.7B" for qwen, "1B"/"3B" for tada as appropriate), then update the
routes/handlers that currently default model_size to "1.7B" (including the logic
referenced around the existing _resolve_generation_engine and the blocks at the
other handler locations) to call this helper with (data, profile), first resolve
the engine via _resolve_generation_engine, and if data.model_size is missing use
the helper-provided fallback for that engine so TADA receives a valid size when
auto-selected. Ensure the helper is referenced by the functions handling
generation requests so both handlers reuse the same fallback logic.
In `@backend/services/profiles.py`:
- Around line 105-110: The designed-profile branch currently only validates
design_prompt and allows creating a designed profile bound to any backend later;
update the validation in the profiles.py logic handling voice_type == "designed"
(and the similar branch around lines 135-139) to require a non-empty
default_engine and to reject creation/update if default_engine is missing or
does not match the engine/backend being requested; also keep rejecting
preset_engine or preset_voice_id for designed profiles and ensure
create_voice_prompt_for_profile() will only receive designed profiles when
default_engine matches the target engine. Ensure you reference and validate the
default_engine field against the current requested engine parameter and return
an error string like the existing checks when mismatches occur.
- Around line 72-83: The helper _get_preset_voice_ids currently returns an empty
set for unknown engines which silently allows invalid "preset" profiles; change
_get_preset_voice_ids to raise a clear exception (e.g., ValueError("unsupported
preset engine: {engine}")) when the engine is not one of the known preset
backends instead of returning set(), and update the code paths that call
_get_preset_voice_ids (the places that currently receive its return and treat an
empty set as "skip validation") to not treat an exception as a skip but to
propagate or surface the error so profile creation/validation fails for
unsupported engines and invalid preset_voice_id values. Ensure references to the
function name _get_preset_voice_ids and any local variable like preset_ids /
voice_type are used to locate and change the behavior.
- Around line 369-383: Normalize the incoming default_engine value (convert
empty string "" to None) before calling _validate_profile_fields so validation
uses the same normalized value later used when updating preset profiles;
specifically update the logic around voice_type, preset_engine, preset_voice_id,
design_prompt and default_engine so default_engine = data.default_engine if
data.default_engine is not None else getattr(profile, "default_engine", None) is
normalized (treat "" as None) prior to invoking _validate_profile_fields(...) to
prevent an empty-string default_engine from passing validation and later
clearing a locked preset profile engine.
---
Outside diff comments:
In `@backend/routes/profiles.py`:
- Around line 1-17: The update_profile_effects function calls datetime.utcnow()
but datetime isn't imported, causing a NameError; add the missing import by
adding "from datetime import datetime" to the module imports at the top so
update_profile_effects can reference datetime.utcnow() (look for the
update_profile_effects function and the top import block to place the import).
---
Duplicate comments:
In `@app/src/components/Generation/GenerationForm.tsx`:
- Around line 30-35: Update getEngineSelectValue to stop unconditionally mapping
'tada' to 'tada:1B' — instead accept an optional preferredTadaSize and/or
profileLanguage and use those to pick the TADA size: if preferredTadaSize is
provided return `tada:{preferredTadaSize}`, else if profileLanguage exists and
!== 'en' return 'tada:3B', else fall back to 'tada:1B' (or preserve the bare
'tada' if you prefer to persist the profile value). Apply the same change to the
other helper instance mentioned (the second getEngineSelectValue occurrence) so
selecting a non-English TADA profile no longer resets language to English.
In `@app/src/components/VoiceProfiles/ProfileForm.tsx`:
- Around line 367-374: The effect clearing defaultEngine to '' prevents
selection of an incompatible engine but the save path collapses '' to undefined
so the backend retains the old default_engine; update the save/edit payload
builder (the function that constructs the edit payload—e.g., the
submit/handleSave or updateVoiceProfile code) to explicitly send a null (or an
explicit empty value) when defaultEngine === '' (for example set
payload.default_engine = defaultEngine === '' ? null : defaultEngine) so the
backend will clear the stored value; apply the same change for the other save
path referenced around the defaultEngine handling at lines 476-483.
In `@backend/routes/generations.py`:
- Around line 43-46: The current except ValueError handlers around
profiles.validate_profile_engine(profile, engine) catch the ValueError and raise
an HTTPException but drop the original exception context; update both handlers
(the one at the validate_profile_engine call and the similar block at lines
around 243-246) to re-raise the HTTPException using "raise HTTPException(... )
from e" so the original ValueError is preserved as the __cause__ for better
debugging while keeping the same status_code and detail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c4a45a6-d548-4208-b6b6-7453b2a1cc19
📒 Files selected for processing (20)
app/src/components/Generation/EngineModelSelector.tsxapp/src/components/Generation/GenerationForm.tsxapp/src/components/ServerSettings/ModelManagement.tsxapp/src/components/VoiceProfiles/ProfileCard.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/components/VoiceProfiles/ProfileList.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/constants/languages.tsapp/src/lib/hooks/useGenerationForm.tsbackend/backends/__init__.pybackend/backends/qwen_custom_voice_backend.pybackend/build_binary.pybackend/database/migrations.pybackend/models.pybackend/requirements.txtbackend/routes/generations.pybackend/routes/profiles.pybackend/services/profiles.pybackend/voicebox-server.spec
💤 Files with no reviewable changes (1)
- app/src/lib/api/client.ts
✅ Files skipped from review due to trivial changes (3)
- backend/requirements.txt
- app/src/components/VoiceProfiles/ProfileList.tsx
- app/src/lib/constants/languages.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/components/ServerSettings/ModelManagement.tsx
- app/src/components/VoiceProfiles/ProfileCard.tsx
- backend/build_binary.py
- backend/models.py
- backend/database/migrations.py
- app/src/lib/hooks/useGenerationForm.ts
- backend/voicebox-server.spec
| async def load_model_async(self, model_size: Optional[str] = None) -> None: | ||
| if model_size is None: | ||
| model_size = self.model_size | ||
|
|
||
| if self.model is not None and self._current_model_size == model_size: | ||
| return | ||
|
|
||
| if self.model is not None and self._current_model_size != model_size: | ||
| self.unload_model() | ||
|
|
||
| await asyncio.to_thread(self._load_model_sync, model_size) | ||
|
|
There was a problem hiding this comment.
Serialize model swaps against in-flight generation.
load_model_async() / unload_model() mutate self.model without synchronization, while generate() dereferences self.model inside a worker thread. Concurrent cold starts can double-load the checkpoint, and a size switch or unload can null out the model mid-request. Guard load/unload/generate with a shared async lock or active-op counter, and pass a stable model reference into _generate_sync.
Based on learnings: In backend/backends/pytorch_backend.py (PyTorchTTSBackend), the model reload/unload race condition requires an asyncio.Lock or active-ops counter to serialize model swap against in-flight inference.
Also applies to: 124-133, 184-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/backends/qwen_custom_voice_backend.py` around lines 82 - 93,
load/unload/generate currently mutate and read self.model without
synchronization causing race conditions; add a shared asyncio.Lock (or an
active-ops counter with a lock) on the backend class and use it to serialize
model swaps and protect in-flight inference: acquire the lock (or increment
active-ops) in load_model_async and unload_model around mutations to self.model
and _current_model_size, and in generate acquire the lock (or increment
active-ops) briefly to capture a stable reference to self.model into a local
variable which you then pass to _generate_sync; update _generate_sync signature
to accept the model reference so threads use the stable snapshot and ensure the
lock/active-ops is released (or decremented) appropriately to avoid deadlocks.
| def _get_preset_voice_ids(engine: str) -> set[str]: | ||
| if engine == "kokoro": | ||
| from ..backends.kokoro_backend import KOKORO_VOICES | ||
|
|
||
| return {voice_id for voice_id, _name, _gender, _lang in KOKORO_VOICES} | ||
|
|
||
| if engine == "qwen_custom_voice": | ||
| from ..backends.qwen_custom_voice_backend import QWEN_CUSTOM_VOICES | ||
|
|
||
| return {voice_id for voice_id, _name, _gender, _lang, _desc in QWEN_CUSTOM_VOICES} | ||
|
|
||
| return set() |
There was a problem hiding this comment.
Reject unsupported preset engines instead of accepting them silently.
Line 83 returns an empty set for unknown engines, and Lines 100-102 treat that as “skip validation”. That lets callers create voice_type="preset" profiles for engines like qwen with arbitrary preset_voice_ids, so the wrong backend can later receive preset metadata it does not implement. Fail closed when _get_preset_voice_ids() returns no entries.
Suggested fix
available_voice_ids = _get_preset_voice_ids(preset_engine)
- if available_voice_ids and preset_voice_id not in available_voice_ids:
+ if not available_voice_ids:
+ return f"Engine '{preset_engine}' does not support preset voice profiles"
+ if preset_voice_id not in available_voice_ids:
return f"Preset voice '{preset_voice_id}' is not valid for engine '{preset_engine}'"
return NoneAlso applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/profiles.py` around lines 72 - 83, The helper
_get_preset_voice_ids currently returns an empty set for unknown engines which
silently allows invalid "preset" profiles; change _get_preset_voice_ids to raise
a clear exception (e.g., ValueError("unsupported preset engine: {engine}")) when
the engine is not one of the known preset backends instead of returning set(),
and update the code paths that call _get_preset_voice_ids (the places that
currently receive its return and treat an empty set as "skip validation") to not
treat an exception as a skip but to propagate or surface the error so profile
creation/validation fails for unsupported engines and invalid preset_voice_id
values. Ensure references to the function name _get_preset_voice_ids and any
local variable like preset_ids / voice_type are used to locate and change the
behavior.
| if voice_type == "designed": | ||
| if not design_prompt or not design_prompt.strip(): | ||
| return "Designed profiles require a design_prompt" | ||
| if preset_engine or preset_voice_id: | ||
| return "Designed profiles cannot set preset_engine or preset_voice_id" | ||
| return None |
There was a problem hiding this comment.
Bind designed profiles to a concrete engine.
These branches only validate design_prompt, so a designed profile can be created and then accepted for qwen, kokoro, etc. create_voice_prompt_for_profile() will forward {"voice_type": "designed", ...} to whatever backend was requested. Use the existing default_engine field to persist the owning engine and reject mismatches here.
Suggested fix
if voice_type == "designed":
if not design_prompt or not design_prompt.strip():
return "Designed profiles require a design_prompt"
+ if not default_engine:
+ return "Designed profiles require a default_engine"
if preset_engine or preset_voice_id:
return "Designed profiles cannot set preset_engine or preset_voice_id"
return None
...
if voice_type == "designed":
design_prompt = getattr(profile, "design_prompt", None)
+ default_engine = getattr(profile, "default_engine", None)
if not design_prompt or not design_prompt.strip():
raise ValueError(f"Designed profile {profile.id} is missing design_prompt")
+ if not default_engine:
+ raise ValueError(f"Designed profile {profile.id} is missing default_engine")
+ if engine != default_engine:
+ raise ValueError(
+ f"Designed profile {profile.id} only supports engine '{default_engine}', not '{engine}'"
+ )
returnAlso applies to: 135-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/profiles.py` around lines 105 - 110, The designed-profile
branch currently only validates design_prompt and allows creating a designed
profile bound to any backend later; update the validation in the profiles.py
logic handling voice_type == "designed" (and the similar branch around lines
135-139) to require a non-empty default_engine and to reject creation/update if
default_engine is missing or does not match the engine/backend being requested;
also keep rejecting preset_engine or preset_voice_id for designed profiles and
ensure create_voice_prompt_for_profile() will only receive designed profiles
when default_engine matches the target engine. Ensure you reference and validate
the default_engine field against the current requested engine parameter and
return an error string like the existing checks when mismatches occur.
| voice_type = getattr(profile, "voice_type", None) or "cloned" | ||
| preset_engine = getattr(profile, "preset_engine", None) | ||
| preset_voice_id = getattr(profile, "preset_voice_id", None) | ||
| design_prompt = getattr(profile, "design_prompt", None) | ||
| default_engine = data.default_engine if data.default_engine is not None else getattr(profile, "default_engine", None) | ||
|
|
||
| validation_error = _validate_profile_fields( | ||
| voice_type=voice_type, | ||
| preset_engine=preset_engine, | ||
| preset_voice_id=preset_voice_id, | ||
| design_prompt=design_prompt, | ||
| default_engine=default_engine, | ||
| ) | ||
| if validation_error: | ||
| raise ValueError(validation_error) |
There was a problem hiding this comment.
Normalize default_engine before validating preset updates.
Line 373 validates the raw request value, but Line 389 later normalizes "" to None. For preset profiles that means default_engine="" passes validation and then clears the engine lock, even though backend/database/models.py marks default_engine as locked for preset profiles.
Suggested fix
- default_engine = data.default_engine if data.default_engine is not None else getattr(profile, "default_engine", None)
+ default_engine = (
+ data.default_engine
+ if data.default_engine is not None
+ else getattr(profile, "default_engine", None)
+ ) or None
+ if voice_type == "preset" and default_engine is None:
+ default_engine = preset_engine
...
- if data.default_engine is not None:
- profile.default_engine = data.default_engine or None # empty string → NULL
+ if data.default_engine is not None:
+ profile.default_engine = default_engine🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/profiles.py` around lines 369 - 383, Normalize the incoming
default_engine value (convert empty string "" to None) before calling
_validate_profile_fields so validation uses the same normalized value later used
when updating preset profiles; specifically update the logic around voice_type,
preset_engine, preset_voice_id, design_prompt and default_engine so
default_engine = data.default_engine if data.default_engine is not None else
getattr(profile, "default_engine", None) is normalized (treat "" as None) prior
to invoking _validate_profile_fields(...) to prevent an empty-string
default_engine from passing validation and later clearing a locked preset
profile engine.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Line 411: The floating flow removed the selectedProfile prop from
EngineModelSelector causing profile-aware engine filtering and auto-correction
to be skipped; restore passing selectedProfile from the surrounding form/state
into EngineModelSelector (same prop used by GenerationForm) so
EngineModelSelector receives selectedProfile and resumes compatibility checks
and auto-corrections for engine/profile combinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 751d81b5-e44b-4702-9a4f-858a4bb8d298
📒 Files selected for processing (1)
app/src/components/Generation/FloatingGenerateBox.tsx
|
|
||
| <FormItem className="flex-1 space-y-0"> | ||
| <EngineModelSelector form={form} compact selectedProfile={selectedProfile} /> | ||
| <EngineModelSelector form={form} compact /> |
There was a problem hiding this comment.
Restore selectedProfile in EngineModelSelector to keep profile-engine compatibility checks active.
Dropping selectedProfile here bypasses profile-aware engine filtering and auto-correction in the floating flow, so users can keep incompatible engine/profile combinations (behavior now differs from GenerationForm).
🔧 Proposed fix
- <EngineModelSelector form={form} compact />
+ <EngineModelSelector form={form} compact selectedProfile={selectedProfile} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <EngineModelSelector form={form} compact /> | |
| <EngineModelSelector form={form} compact selectedProfile={selectedProfile} /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/Generation/FloatingGenerateBox.tsx` at line 411, The
floating flow removed the selectedProfile prop from EngineModelSelector causing
profile-aware engine filtering and auto-correction to be skipped; restore
passing selectedProfile from the surrounding form/state into EngineModelSelector
(same prop used by GenerationForm) so EngineModelSelector receives
selectedProfile and resumes compatibility checks and auto-corrections for
engine/profile combinations.
3eccdf4 to
e6f419c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/backends/__init__.py (1)
418-423:⚠️ Potential issue | 🟠 MajorInvalid
qwen_custom_voicesizes still escape as 500s here.
QwenCustomVoiceBackend._is_model_cached()calls_get_model_path(), which raisesValueErrorfor unknown sizes. With the current request schema still accepting"1B"/"3B", a streaming request likeengine="qwen_custom_voice", model_size="3B"reaches this branch and fails before the intendedHTTPExceptionis raised. CatchValueErrorhere or validate the engine/size pair before calling_is_model_cached().💡 Possible fix
if engine in ("qwen", "qwen_custom_voice", "tada"): - if not backend._is_model_cached(model_size): + try: + is_cached = backend._is_model_cached(model_size) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) from e + if not is_cached: raise HTTPException( status_code=400, detail=f"Model {model_size} is not downloaded yet. Use /generate to trigger a download.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/backends/__init__.py` around lines 418 - 423, The branch that calls backend._is_model_cached() can raise ValueError from QwenCustomVoiceBackend._get_model_path() for invalid sizes (e.g., "3B"), so wrap the call in a try/except or validate the engine/model_size pair first: catch ValueError around backend._is_model_cached(model_size) (or pre-check that engine == "qwen_custom_voice" and model_size is one of the supported sizes) and convert the error into the same HTTPException used for missing models (status_code=400, descriptive detail) so invalid sizes return 400 instead of bubbling up as 500.
♻️ Duplicate comments (3)
backend/database/migrations.py (1)
197-198:⚠️ Potential issue | 🟠 MajorRemove CWD fallback when rebased path is missing.
At Line 233, falling back to
p.resolve()makes migration behavior depend on process CWD (which is not controlled at startup inbackend/database/session.py,backend/app.py, andbackend/main.py). This can silently rewrite DB rows to wrong files when a same-named relative path exists under CWD.🔧 Proposed fix
- Strategy: paths like "data/generations/abc.wav" are rebased onto the - configured data directory. If the path starts with "data/", strip that - prefix and prepend get_data_dir(). Otherwise, join the relative path - directly under get_data_dir(). If the rebased path still does not exist, - fall back to resolving relative to CWD. + Strategy: paths like "data/generations/abc.wav" are rebased onto the + configured data directory. If the path starts with "data/", strip that + prefix and prepend get_data_dir(). Otherwise, join the relative path + directly under get_data_dir(). Only existing rebased paths are migrated. @@ - resolved = rebased if rebased.exists() else p.resolve() - if resolved.exists(): + if rebased.exists(): conn.execute( text(f"UPDATE {table} SET {column} = :path WHERE id = :id"), - {"path": str(resolved), "id": row_id}, + {"path": str(rebased), "id": row_id}, ) total_fixed += 1Also applies to: 233-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/database/migrations.py` around lines 197 - 198, The code currently falls back to calling p.resolve() when a rebased path (computed relative to get_data_dir()) doesn't exist, which makes behavior depend on the process CWD; remove that CWD fallback by deleting the p.resolve() branch and instead treat a missing rebased path as an error (e.g., raise FileNotFoundError or return an explicit None) so rows are not silently rewritten to files under CWD; update the logic around get_data_dir() and the rebasing code that invokes p.resolve() to either raise a clear error with context or return the missing-path marker instead of resolving against CWD.app/src/components/Generation/FloatingGenerateBox.tsx (1)
411-411:⚠️ Potential issue | 🟠 MajorShowing all engines here still needs a profile-compatibility guard.
Without
selectedProfile, Line 411 can switch the form toqwen_custom_voicewhile a cloned profile is selected, or back to a cloning engine while a preset profile is selected.backend/routes/generations.pynow rejects those combinations withprofiles.validate_profile_engine(), so the floating box can enter a state that always 400s — especially whenshowVoiceSelectoris false. Keep the full engine list if that's intentional, but auto-switch/clear the profile or block submit when the pair is incompatible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/FloatingGenerateBox.tsx` at line 411, The EngineModelSelector in FloatingGenerateBox can let the form switch to engines incompatible with the current selectedProfile, causing backend 400s via profiles.validate_profile_engine; update FloatingGenerateBox to check compatibility whenever EngineModelSelector changes or when selectedProfile changes (use the form state and selectedProfile prop), and if the new engine/profile pair is incompatible either auto-clear selectedProfile (or auto-switch the engine to a compatible default) or disable the submit button and prevent selection of incompatible engines when showVoiceSelector is false; reference the EngineModelSelector component, the form state fields (engine, selectedProfile), and the profiles.validate_profile_engine check so you enforce the guard in the selection handler and on submit.app/src/components/Generation/GenerationForm.tsx (1)
30-35:⚠️ Potential issue | 🟠 Major
tadanormalization still overrides non-English profile languageLine 33 always maps bare
tadatotada:1B, and Line 54 applies it immediately; this can still reset non-English profiles to English via engine selection side effects. Make this mapping language-aware at the helper boundary and passselectedProfile.languagefrom the caller.Suggested fix
-function getEngineSelectValue(engine: string): string { +function getEngineSelectValue(engine: string, profileLanguage?: string): string { if (engine === 'qwen') return 'qwen:1.7B'; if (engine === 'qwen_custom_voice') return 'qwen_custom_voice:1.7B'; - if (engine === 'tada') return 'tada:1B'; + if (engine === 'tada') return profileLanguage === 'en' ? 'tada:1B' : 'tada:3B'; return engine; } @@ - applyEngineSelection(form, getEngineSelectValue(preferredEngine)); + applyEngineSelection( + form, + getEngineSelectValue(preferredEngine, selectedProfile.language), + );Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/GenerationForm.tsx` around lines 30 - 35, The helper getEngineSelectValue currently normalizes 'tada' to 'tada:1B' unconditionally, which can override non-English profile language; change getEngineSelectValue(engine: string) to getEngineSelectValue(engine: string, profileLanguage?: string) and only map bare 'tada' -> 'tada:1B' (and similarly for 'qwen'/'qwen_custom_voice' if needed) when profileLanguage indicates English (e.g., 'en'); update all callers (the engine selection usage around the existing engine mapping/selection code that currently calls getEngineSelectValue, including where selectedProfile.language is in scope) to pass selectedProfile.language, and leave the function to return the original engine unchanged for non-English profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/VoiceProfiles/ProfileCard.tsx`:
- Around line 20-24: ENGINE_DISPLAY_NAMES in ProfileCard.tsx uses "CustomVoice"
for the key qwen_custom_voice which mismatches the "Qwen CustomVoice" label
rendered by ProfileList.tsx; fix by centralizing the engine display names
(exporting/importing a shared map) or change
ENGINE_DISPLAY_NAMES['qwen_custom_voice'] to "Qwen CustomVoice" so both
ProfileCard and ProfileList use the identical label (update the constant named
ENGINE_DISPLAY_NAMES or replace its usage with the shared map).
In `@backend/backends/qwen_custom_voice_backend.py`:
- Around line 188-193: The seeded RNG calls in _generate_sync currently use
torch.manual_seed and torch.cuda.manual_seed which mutate global PyTorch state
and cause cross-request interference; replace this by either (preferred)
creating and using a per-request torch.Generator (pass it into the model/sample
calls if Qwen API accepts a generator) or, if the API doesn't support a
Generator, wrap the seed section in a backend-level lock or save the global RNG
state with torch.get_rng_state()/torch.cuda.get_rng_state_all(), set the seeds,
run generation, then restore the saved RNG states using
torch.set_rng_state()/torch.cuda.set_rng_state_all() to avoid leaking the seed
to other threads. Ensure changes target the _generate_sync function and related
generation calls that currently rely on
torch.manual_seed/torch.cuda.manual_seed.
In `@backend/routes/generations.py`:
- Around line 23-24: The resolver _resolve_generation_engine is never reaching
profile defaults because models.GenerationRequest currently defaults engine to
"qwen"; either make the request fields nullable (change GenerationRequest.engine
and .model_size to Optional[str] with no literal default) or change the resolver
to detect whether the caller omitted the field by checking data.model_fields_set
(or equivalent) before using data.engine; update _resolve_generation_engine to
use: if "engine" in data.model_fields_set then use data.engine else fall through
to getattr(profile, "default_engine", None) or getattr(profile, "preset_engine",
None) or "qwen" (and apply the same pattern for model_size defaults), so
profile-aware auto-selection and engine-specific size defaults are respected.
---
Outside diff comments:
In `@backend/backends/__init__.py`:
- Around line 418-423: The branch that calls backend._is_model_cached() can
raise ValueError from QwenCustomVoiceBackend._get_model_path() for invalid sizes
(e.g., "3B"), so wrap the call in a try/except or validate the engine/model_size
pair first: catch ValueError around backend._is_model_cached(model_size) (or
pre-check that engine == "qwen_custom_voice" and model_size is one of the
supported sizes) and convert the error into the same HTTPException used for
missing models (status_code=400, descriptive detail) so invalid sizes return 400
instead of bubbling up as 500.
---
Duplicate comments:
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Line 411: The EngineModelSelector in FloatingGenerateBox can let the form
switch to engines incompatible with the current selectedProfile, causing backend
400s via profiles.validate_profile_engine; update FloatingGenerateBox to check
compatibility whenever EngineModelSelector changes or when selectedProfile
changes (use the form state and selectedProfile prop), and if the new
engine/profile pair is incompatible either auto-clear selectedProfile (or
auto-switch the engine to a compatible default) or disable the submit button and
prevent selection of incompatible engines when showVoiceSelector is false;
reference the EngineModelSelector component, the form state fields (engine,
selectedProfile), and the profiles.validate_profile_engine check so you enforce
the guard in the selection handler and on submit.
In `@app/src/components/Generation/GenerationForm.tsx`:
- Around line 30-35: The helper getEngineSelectValue currently normalizes 'tada'
to 'tada:1B' unconditionally, which can override non-English profile language;
change getEngineSelectValue(engine: string) to getEngineSelectValue(engine:
string, profileLanguage?: string) and only map bare 'tada' -> 'tada:1B' (and
similarly for 'qwen'/'qwen_custom_voice' if needed) when profileLanguage
indicates English (e.g., 'en'); update all callers (the engine selection usage
around the existing engine mapping/selection code that currently calls
getEngineSelectValue, including where selectedProfile.language is in scope) to
pass selectedProfile.language, and leave the function to return the original
engine unchanged for non-English profiles.
In `@backend/database/migrations.py`:
- Around line 197-198: The code currently falls back to calling p.resolve() when
a rebased path (computed relative to get_data_dir()) doesn't exist, which makes
behavior depend on the process CWD; remove that CWD fallback by deleting the
p.resolve() branch and instead treat a missing rebased path as an error (e.g.,
raise FileNotFoundError or return an explicit None) so rows are not silently
rewritten to files under CWD; update the logic around get_data_dir() and the
rebasing code that invokes p.resolve() to either raise a clear error with
context or return the missing-path marker instead of resolving against CWD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff9c3efe-7002-40b7-a449-3a7ff013834e
📒 Files selected for processing (21)
app/src/components/Generation/EngineModelSelector.tsxapp/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/Generation/GenerationForm.tsxapp/src/components/ServerSettings/ModelManagement.tsxapp/src/components/VoiceProfiles/ProfileCard.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/components/VoiceProfiles/ProfileList.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/constants/languages.tsapp/src/lib/hooks/useGenerationForm.tsbackend/backends/__init__.pybackend/backends/qwen_custom_voice_backend.pybackend/build_binary.pybackend/database/migrations.pybackend/models.pybackend/requirements.txtbackend/routes/generations.pybackend/routes/profiles.pybackend/services/profiles.pybackend/voicebox-server.spec
💤 Files with no reviewable changes (1)
- app/src/lib/api/client.ts
✅ Files skipped from review due to trivial changes (3)
- backend/requirements.txt
- app/src/lib/constants/languages.ts
- backend/models.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/build_binary.py
- app/src/components/ServerSettings/ModelManagement.tsx
- app/src/lib/api/types.ts
- app/src/lib/hooks/useGenerationForm.ts
- backend/routes/profiles.py
- backend/voicebox-server.spec
- backend/services/profiles.py
| /** Human-readable display names for preset engine badges. */ | ||
| const ENGINE_DISPLAY_NAMES: Record<string, string> = { | ||
| kokoro: 'Kokoro', | ||
| qwen_custom_voice: 'CustomVoice', | ||
| }; |
There was a problem hiding this comment.
Use the same qwen_custom_voice label across profile surfaces.
ProfileList.tsx renders this engine as "Qwen CustomVoice", but the badge here renders "CustomVoice". The same preset type ends up with two names in adjacent UI. Reuse a shared engine-name map or align this label with the list.
Also applies to: 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/VoiceProfiles/ProfileCard.tsx` around lines 20 - 24,
ENGINE_DISPLAY_NAMES in ProfileCard.tsx uses "CustomVoice" for the key
qwen_custom_voice which mismatches the "Qwen CustomVoice" label rendered by
ProfileList.tsx; fix by centralizing the engine display names
(exporting/importing a shared map) or change
ENGINE_DISPLAY_NAMES['qwen_custom_voice'] to "Qwen CustomVoice" so both
ProfileCard and ProfileList use the identical label (update the constant named
ENGINE_DISPLAY_NAMES or replace its usage with the shared map).
| def _generate_sync(): | ||
| if seed is not None: | ||
| torch.manual_seed(seed) | ||
| if torch.cuda.is_available(): | ||
| torch.cuda.manual_seed(seed) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PyTorch, do torch.manual_seed()andtorch.cuda.manual_seed() change RNG state shared across threads, and what is the recommended way to keep concurrent inference requests deterministic per request?
💡 Result:
No, torch.manual_seed and torch.cuda.manual_seed do not change thread-local RNG states; they set the global, process-wide RNG state shared across all threads. This shared state means concurrent inference requests can interfere with each other's RNG if any random operations are used, leading to non-determinism across requests. The recommended way to achieve determinism per concurrent inference request is to create and use a dedicated torch.Generator instance for each request (or thread), seeded consistently (e.g., with a fixed seed or request-specific value). Pass this generator explicitly to any PyTorch random operations (e.g., torch.rand(..., generator=gen)). For operations without a generator argument, save/restore the global RNG state around the request using torch.random.get_rng_state and torch.random.set_rng_state (CPU) or equivalent CUDA methods, though generators are preferred where possible. Additionally, for full determinism: - Call torch.use_deterministic_algorithms(True) and torch.backends.cudnn.deterministic = True. - Set torch.backends.cudnn.benchmark = False. - Use torch.inference_mode or model.eval. - Avoid nondeterministic ops; check PyTorch reproducibility docs for details. PyTorch inference is thread-safe (models are const during forward), so concurrent requests on the same model instance are safe, but RNG requires explicit isolation via generators or state management.
Citations:
- 1: https://pytorch.org/docs/stable/notes/randomness.html
- 2: https://docs.pytorch.org/docs/main/random.html
- 3: https://pytorch.org/docs/stable/generated/torch.cuda.manual_seed.html
- 4: https://docs.pytorch.org/docs/stable/generated/torch.Generator.html
- 5: Do not modify global random state pytorch/pytorch#39716
- 6: https://runebook.dev/en/docs/pytorch/generated/torch.generator/torch.Generator
- 7: https://pytorch.org/docs/stable/generated/torch.use_deterministic_algorithms.html
- 8: https://docs.pytorch.org/docs/2.5/notes/randomness.html
- 9: https://docs.pytorch.org/docs/2.5/notes/cpu_threading_torchscript_inference.html
- 10: https://www.reddit.com/r/MachineLearning/comments/1d2dsz1/d_how_to_run_concurrent_inferencing_on_pytorch/
Concurrent seeded requests interfere due to global RNG state
torch.manual_seed() and torch.cuda.manual_seed() set process-wide PyTorch RNG state. When multiple requests call _generate_sync() concurrently in worker threads, each seeding call overwrites the others, breaking per-request determinism.
Fix by creating a dedicated torch.Generator per request (if Qwen's API supports it), protecting the seeding section with a backend-level lock, or saving/restoring global RNG state around the seeded section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/backends/qwen_custom_voice_backend.py` around lines 188 - 193, The
seeded RNG calls in _generate_sync currently use torch.manual_seed and
torch.cuda.manual_seed which mutate global PyTorch state and cause cross-request
interference; replace this by either (preferred) creating and using a
per-request torch.Generator (pass it into the model/sample calls if Qwen API
accepts a generator) or, if the API doesn't support a Generator, wrap the seed
section in a backend-level lock or save the global RNG state with
torch.get_rng_state()/torch.cuda.get_rng_state_all(), set the seeds, run
generation, then restore the saved RNG states using
torch.set_rng_state()/torch.cuda.set_rng_state_all() to avoid leaking the seed
to other threads. Ensure changes target the _generate_sync function and related
generation calls that currently rely on
torch.manual_seed/torch.cuda.manual_seed.
| def _resolve_generation_engine(data: models.GenerationRequest, profile) -> str: | ||
| return data.engine or getattr(profile, "default_engine", None) or getattr(profile, "preset_engine", None) or "qwen" |
There was a problem hiding this comment.
This resolver can't auto-select while engine still defaults to "qwen".
backend/models.py still fills omitted engine / model_size with "qwen" / "1.7B", so data.engine or ... never falls through to profile.default_engine / profile.preset_engine. The new profile-aware auto-selection and engine-specific size defaults are bypassed for callers that omit those fields. Make those request fields nullable, or inspect model_fields_set before resolving defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/generations.py` around lines 23 - 24, The resolver
_resolve_generation_engine is never reaching profile defaults because
models.GenerationRequest currently defaults engine to "qwen"; either make the
request fields nullable (change GenerationRequest.engine and .model_size to
Optional[str] with no literal default) or change the resolver to detect whether
the caller omitted the field by checking data.model_fields_set (or equivalent)
before using data.engine; update _resolve_generation_engine to use: if "engine"
in data.model_fields_set then use data.engine else fall through to
getattr(profile, "default_engine", None) or getattr(profile, "preset_engine",
None) or "qwen" (and apply the same pattern for model_size defaults), so
profile-aware auto-selection and engine-specific size defaults are respected.
Summary
Notes
qwen-ttsdependency, so no new Python package install path was neededSummary by CodeRabbit
New Features
Improvements
Removals